This repository has been archived by the owner on Apr 22, 2023. It is now read-only.
v8: fix let bindings in for loops #23948
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The test LGTM. The floating patch appears to be correct. |
|
Thank you @cjihrig! |
Backport b17eaaa5755e625493c5fe537f42b58838923c52 from upstream v8. Original commit message: Fix desugaring of let bindings in for loops to handle continue properly This requires putting the original loop's body inside an inner for loop (with the same labels as the original loop) and re-binding the temp variables in its "next" expression. A second flag is added to the desugared code to ensure the loop body executes at most once per loop. BUG=v8:3683 LOG=y Review URL: https://codereview.chromium.org/720863002 Cr-Commit-Position: refs/heads/master@{nodejs#25363} Fixes nodejs#9113 and nodejs#14411.
0724602 floats a patch on V8 that fixes issue nodejs#9113 that would cause let bindings and continue statements in for loops to not work properly. This change adds a regression test that fails if that patch is not properly floated, thus preventing us from not floating that patch after future V8 upgrades.
misterdjules
force-pushed
the
fix-issue-9113
branch
from
00ca880
to
8d1baac
Compare
|
Rebased on top of v0.12, landing soon. |
misterdjules
pushed a commit
that referenced
this pull request
Apr 30, 2015
Backport b17eaaa5755e625493c5fe537f42b58838923c52 from upstream v8. Original commit message: Fix desugaring of let bindings in for loops to handle continue properly This requires putting the original loop's body inside an inner for loop (with the same labels as the original loop) and re-binding the temp variables in its "next" expression. A second flag is added to the desugared code to ensure the loop body executes at most once per loop. BUG=v8:3683 LOG=y Review URL: https://codereview.chromium.org/720863002 Cr-Commit-Position: refs/heads/master@{#25363} Fixes #9113 and #14411. Reviewed-By: Colin Ihrig <cjihrig@gmail.com> PR-URL: #23948
misterdjules
pushed a commit
that referenced
this pull request
Apr 30, 2015
0724602 floats a patch on V8 that fixes issue #9113 that would cause let bindings and continue statements in for loops to not work properly. This change adds a regression test that fails if that patch is not properly floated, thus preventing us from not floating that patch after future V8 upgrades. Reviewed-By: Colin Ihrig <cjihrig@gmail.com> PR-URL: #23948
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
They are in two separate commits so that every time we float the patch, we don't float the test too.
This PR is based off another PR that downgrades V8 to the actual proper stable version for node v0.12, which is the reason why there are more commits than the two highlighted above.
Once #18206 lands, this PR will be rebased on top of v0.12 and will only include these two commits.
Fixes #9113 and #14411.
/cc @joyent/node-coreteam and especially @mhdawson.